fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module #501
Conversation
|
This is now ready for review and could potentially be released as a new major. The rollup peer dependency has been updated to |
|
Apparently some local paths were re-introduced into the bundle, I will need to check that. Back to draft it is. |
|
Ok, all fixed now, please have a look. I very much recommend studying the diff of the function test snapshots at https://github.com/rollup/plugins/pull/501/files#diff-b197c1539f3dea1e33fceb0a1ace63f3 as they can explain the nature of the changes better, and also take a look at the added tests. |
BREAKING CHANGES: updates rollup peerDependency to ^2.22.0
BREAKING CHANGES: CJS entry points will always have a default export
lukastaegert
force-pushed
the
commonjs-better-synthetic-named-exports
branch
from
149224c
to
d8e3a6f
Compare
| @@ -1,4 +1,4 @@ | |||
| const nodeResolve = require('@rollup/plugin-node-resolve'); | |||
| const { nodeResolve } = require('@rollup/plugin-node-resolve'); | |||
There was a problem hiding this comment.
Why have you switched to importing the named export in favor of the default export?
Is it just to be explicit or didn't it work anymore with the default export?
I know that it exports both, but we need to be sure that nothing has been fatally broken :-)
There was a problem hiding this comment.
This is not an import, this is a CJS require. Since a recent version, this plugin switched to named export mode. We could also have used the default export at .default but then if we need to use a property anyway, I prefer the named export.
|
This will be difficult to test with all situations people has experienced out there. Can we tag some people who reported issues so they'll test it live? |
| ␊ | ||
| };␊ | ||
| ␊ | ||
| module.exports = main;␊ |
There was a problem hiding this comment.
Practically there's no difference here right? As it's an empty object vs the default empty exports object...
There was a problem hiding this comment.
Good point. This could be an optimization to add.
There was a problem hiding this comment.
Actually, this could be problematic to get rid of because this plugin converts to ESM and that one does not have an implicit default export. So an empty object is generated. I do not see how Rollup core could get rid of this easily later. I think this would be a job to be fixed manually via an exposedExports option.
shellscape
changed the title
shellscape
requested review from
danielgindi,
NotWoods,
Andarist,
guybedford,
LarsDenBakker and
Rich-Harris
|
If anyone is interested: In order to test this, you can clone the repo, run |
|
I can definitely confirm that this PR solves the issue described in #411. The generated import test2 from './test2.js';
console.log(test2);
var test1 = 1;
export default test1;without a reference to |
lukastaegert
changed the title
|
I can confirm that #454 is also resolved by this PR. |
|
#431 is still broken. I might have a look at this one. |
|
#431 seems to require a lot of setup to reproduce, I guess it should be addressed in a separate PR. So from my side, I still think this one is good to go. |
|
@lukastaegert I can hold off on publishing this if changes for the other issues you mentioned will also be breaking |
|
They will, the question is only when they will be ready... |
|
OK. Let's sit on this one for a short while in the event we get a few more in. |
|
FWIW I disagree with this approach since it does not match Node.js interop and likely never will since Node.js support for named exports is now fully stalled after three separate failed initatives over many years. |
|
Noted. But at least for bundlers, handling ES modules that were transpiled to CJS in the same way as the original ES module is very important. This PR fixes the default export handling and brings it more in line with how other bundlers handle it. Named exports existed before and after this PR and were not touched here. |
|
It would help to have a flag to change this behaviour for users who want to bundle Node.js apps. I'm sure an issue will come up at some point in the coming months to track this by someone. |
|
Sure, as it has been discussed before, and I very much agree we should have it. Forbidding named exports and preventing interop however is only a small part of this, properly handling |
|
If we add this, then it should certainly be at least close to the full package, otherwise I am not sure users will see the benefit. |
|
What information from the package is required from the commonjs plugin
perspective?
It should be just the format info in the ambiguous cases, which could in
the worst case be loaded twice with a 15 line package.json boundary check
and load and rely on the process-level file system caching of the operating
system.
…On Tue, 21 Jul 2020 at 08:47, Lukas Taegert-Atkinson < ***@***.***> wrote:
If we add this, then it should certainly be at least close to the full
package, otherwise I am not sure users will see the benefit.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#501 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAESFSX5ITVB6K2IODCDDZLR4W2AZANCNFSM4O6YXUPQ>
.
|
|
Finding and resolving package.json files and in extension libraries relying on them is one of the most performance critical parts of bundling, more performance critical than actually loading the files. The node-resolve plugin has excellent caching to avoid any unnecessary directory lookup while also providing the information which package.json file is responsible for a js file. Would be a waste not to reuse this information. |
… from an actual ESM (rollup#501) * fix(commonjs): create more efficient dynamic require code * chore(commonjs): update dependencies BREAKING CHANGES: updates rollup peerDependency to ^2.22.0 * feat(commonjs): support all styles of transpiled ES modules BREAKING CHANGES: CJS entry points will always have a default export
Due to rollup/plugins#501, from version 15.0.0 onwards the generated addon breaks: the rollup plugin automatically creates an `export default` section at the end of the generated code, which the addon system doesn't really like.
Due to rollup/plugins#501, from version 15.0.0 onwards the generated addon breaks: the rollup plugin automatically creates an `export default` section at the end of the generated code, which the addon system doesn't really like.
Rollup Plugin Name:
commonjsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Part of #481
Resolves #411
Resolves #454
Description
The main part of this PR is to re-introduce proper interop handling when importing transpiled ESM files, i.e. CJS files with
exports.__esModule: true. It now relies on a new Rollup feature to take synthetic named exports from an arbitrary export, in this case__moduleExports. This export will not become part of the public interface of entry points and not show up in reified namespace objects.In contrast to what I wrote in #481, I again used an interop function because due to the PURE annotation, it allowed for better tree-shaking. The interop I used is actually identical to the old one, which means if there is no
defaultproperty, the default export falls back to the namespace. This is not according to specs but is needed to prevent consumers that use Rollup's old interop pattern from failing, see the comment I added in the code and rollup/rollup-plugin-commonjs#224A breaking change I added is that now CJS files will again always expose a default export, even if they are entry points. The reason is that the old logic was very error prone and often did not detect reliably if later dependencies would import something from an entry point. Once we add the
exposedExportsoption I recommended in #481, this should be easily fixable manually by the user. For now, they can just use an ESM wrapper as their entry point.Also, I changed how the helpers are imported when using dynamic requires. More precisely, I added an ad-hoc wrapper that just reexports the
commonjsRegisterhelper as default export and required that from CJS which prevented the creation of an unneeded namespace object and allowed unused helpers to be tree-shaken away.Last, I updated all dependencies as I needed to update the Rollup dependency anyway. Another breaking change is the new peer dependency version of Rollup.
Due to the pressing nature of this fix I would recommend to release rather sooner than later, even though the other fixes in #481 will likely need yet another major version.